Closes #2126 Implement NtQueryInformationProcess to get commandline Windows 8.1+ - #2127
Closes #2126 Implement NtQueryInformationProcess to get commandline Windows 8.1+ #2127Ahm3dRN wants to merge 2 commits into
Conversation
shirou
left a comment
shirou
left a comment
There was a problem hiding this comment.
Thanks for the thorough issue and for digging up the SystemInformer / psutil references — the direction is right and I'd like to take this. Four things before merging.
1. Make the native query a fallback, not the primary path.
Right now it runs first for every Windows process, so everyone's cmdline source becomes an undocumented information class. psutil does the opposite — PEB first, ProcessCommandLineInformation only on permission errors (_pswindows.py, see use_peb) — because a process can be started suspended with its PEB command line patched, and then the PEB value is the one the process actually sees. #2126 only needs the case where the PEB read fails, so a fallback fixes it with zero impact on every other process.
2. Real protected processes aren't reached yet — please cover them here.
getProcessCommandLine opens with PROCESS_QUERY_LIMITED_INFORMATION | PROCESS_VM_READ and returns ("", nil) on ACCESS_DENIED, before getCmdlineProtected is ever called. For PPL processes (services.exe, csrss.exe, MsMpEng.exe, …) that OpenProcess is exactly what fails, so the PR as it stands only helps the "handle opens, but ReadProcessMemory is blocked" case (yours). SystemInformer's doc says PROCESS_QUERY_LIMITED_INFORMATION alone suffices on 8.1+, and psutil opens a separate handle with just that. Since the PR is titled "protected processes", I'd like it to actually reach them:
h, err := windows.OpenProcess(processQueryInformation|windows.PROCESS_VM_READ, false, uint32(pid))
if errors.Is(err, windows.ERROR_ACCESS_DENIED) {
// PPL: VM_READ is refused, but a query-only handle may still open
if lh, lerr := windows.OpenProcess(processQueryInformation, false, uint32(pid)); lerr == nil {
defer windows.CloseHandle(lh)
return getProcessCommandLineNative(lh)
}
return "", nil
}3. Accept the other "buffer too small" statuses. psutil treats STATUS_BUFFER_OVERFLOW and STATUS_BUFFER_TOO_SMALL as success alongside STATUS_INFO_LENGTH_MISMATCH; all three are in golang.org/x/sys/windows. On a machine returning one of the other two, this silently does nothing.
4. Bounds-check the buffer. returnLength == 0 is guarded but 1..15 isn't (sizeof(NTUnicodeString) is 16 on amd64, 8 on 386), and strEnd isn't checked against len(buf) — buf[strOffset:strEnd] can panic, which callers can't recover from. Unlikely in practice, but we're trusting an undocumented API's return value. Reading the length via binary.LittleEndian.Uint16(buf[0:2]) instead of casting to NTUnicodeString also sidesteps golang/go#73460 (the issue your comment links to) entirely.
Smaller points:
- Once it's a fallback, CI never exercises this path. Please split the buffer parsing into a pure
func([]byte) (string, error)and table-test it (valid / zero length / truncated) — that covers 4 too. - As you noted in the issue, SystemInformer allocates a reasonable buffer up front and only retries on
STATUS_INFO_LENGTH_MISMATCH, halving the syscalls. Lower priority once this is a fallback. getCmdlineProtectedsplits thegetUserProcessParams32/64pair — please move it next togetProcessCommandLine. It's a general native query rather than protected-process-specific, sogetProcessCommandLineNativereads better.- Move
processCommandLineInformation = 60next toprocessQueryInformationwith a note that it'sProcessCommandLineInformation(ntpsapi.h, Windows 8.1+). // Try the native command-line query first succeeds on protectedis missing a clause.
1–4 are what I'd like fixed before merging; the smaller points can follow up if you prefer. Thanks again.
|
Thank you so much for the thorough review and for giving my PR time. for Points 1 and 2, I've created a new func then I open a handle with only my current approach that considers both cases where a handle is granted but memory read is blocked and a genuine PPL process func getProcessCommandLine(pid int32) (string, error) {
h, err := windows.OpenProcess(processQueryInformation|windows.PROCESS_VM_READ, false, uint32(pid))
if err == nil {
defer syscall.CloseHandle(syscall.Handle(h))
if cmdLine, err := getProcessCommandLinePEB(h); err == nil {
return cmdLine, nil
}
// PEB read failed even though the handle opened. VM_READ may
// have been granted but ineffective against this process's
// protection driver. Fall back to the native query on the same handle.
if cmdLine, err := getProcessCommandLineNative(h, pid); err == nil {
return cmdLine, nil
}
return "", nil
}
if errors.Is(err, windows.ERROR_INVALID_PARAMETER) {
return "", nil
}
if !errors.Is(err, windows.ERROR_ACCESS_DENIED) {
return "", err
}
// fallback in case it's a genuine PPL process, where
// PROCESS_VM_READ itself is denied at OpenProcess time. Retry with
// just PROCESS_QUERY_LIMITED_INFORMATION, which PPL still grants.
lh, lerr := windows.OpenProcess(processQueryInformation, false, uint32(pid))
if lerr != nil {
return "", nil
}
defer syscall.CloseHandle(syscall.Handle(lh))
return getProcessCommandLineNative(lh, pid)
}Point 3 Done func parseCommandLineInformation(buf []byte) (string, error) {
if len(buf) < 2 {
return "", errors.New("command line buffer too small to read length")
}
length := binary.LittleEndian.Uint16(buf[0:2])
if length == 0 {
return "", nil
}
strOffset := int(unsafe.Sizeof(windows.NTUnicodeString{}))
strEnd := strOffset + int(length)
if strEnd > len(buf) {
return "", errors.New("command line length exceeds buffer size")
}
return convertUTF16ToString(buf[strOffset:strEnd]), nil
}aside from the second smaller point "reasonable buffer allocation" everything else is ready just waiting your confirmation. |
2 participants
Adds
getCmdlineProtected, which usesProcessCommandLineInformation->NtQueryInformationProcessbefore falling back to the existing PEB memory readapproach in
getProcessCommandLine.If it fails (pre-8.1 version of windows or such) it should fall back to the current implementation of PEB.
Tested against
LeagueClient.exea Vanguard-protected process on Windows 10this Closes #2126